feat: broadcast dsq messages using the inventory system #6148

PastaPastaPasta · 2024-07-24T04:02:46Z

Issue being fixed or feature implemented

DSQ messages are 142 bytes.

Previously, assuming a relatively highly connected masternode hosting 100 connection, each round of coinjoin will result in 14.2KB (100*142) of inbound and outbound traffic each.

What was done?

Now, using the inventory system, a message will first use 36 bytes per peer (sending and receiving), plus the size of a getdata message and the actual message itself. As a result, bandwidth usage for 1 round of mixing would be closer to 36 * 100 + 142 (dsq) + 36 (getdata) = ~3.8KB, a reduction of around ~73%

How Has This Been Tested?

Has not been; @UdjinM6 especially please review well :)

Breaking Changes

Does introduce a new protocol version, but in a backwards compatible way. I don't think this would need to be delayed to v22 for any reason.

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

UdjinM6

pls see a few thoughts/suggestions below + linter is complaining

src/protocol.h

src/coinjoin/coinjoin.cpp

PastaPastaPasta · 2024-08-20T20:46:59Z

rebased on develop

PastaPastaPasta · 2024-08-20T21:48:18Z

@UdjinM6 can you re-review based on latest?

UdjinM6 · 2024-08-21T12:54:36Z

@UdjinM6 can you re-review based on latest?

any thoughts on my previous comments? #6148 (review)

PastaPastaPasta · 2024-08-21T15:47:51Z

@UdjinM6 can you re-review based on latest?

any thoughts on my previous comments? #6148 (review)

@UdjinM6 what do you mean? I thought I applied all concepts, didn't fix linter yet. Is that all you mean?

UdjinM6 · 2024-08-21T16:14:42Z

@UdjinM6 can you re-review based on latest?

any thoughts on my previous comments? #6148 (review)

@UdjinM6 what do you mean? I thought I applied all concepts, didn't fix linter yet. Is that all you mean?

Weird... Had to reload a couple of times to see new changes 🤷‍♂️

src/coinjoin/coinjoin.cpp

src/net_processing.cpp

PastaPastaPasta · 2024-08-29T18:05:36Z

@UdjinM6 please re-review

UdjinM6 · 2024-08-30T14:44:57Z

src/version.h

@@ -55,6 +55,9 @@ static const int MNLISTDIFF_CHAINLOCKS_PROTO_VERSION = 70230;
 //! Legacy ISLOCK messages and a corresponding INV were dropped in this version
 static const int NO_LEGACY_ISLOCK_PROTO_VERSION = 70231;

+//! Inventory type for DSQ messages added
+static const int DSQ_INV_VERSION = 70233;


21.1 nodes get "unknown inv" and can't mix, we need a proto bump (70233 is 21.1)

UdjinM6 · 2024-08-30T14:45:32Z

doc/release-notes-6148.md

+P2P and Network Changes
+-----------------------
+
+The DSQ message, starting in protocol version 70233, is broadcast using the inventory system, and not simply


Suggested change

The DSQ message, starting in protocol version 70233, is broadcast using the inventory system, and not simply

The DSQ message, starting in protocol version 70234, is broadcast using the inventory system, and not simply

UdjinM6 · 2024-08-30T14:51:08Z

also, clang-format isn't happy :)

UdjinM6

can't compile, pls see 7170f0e

UdjinM6

ACK 7170f0e

nit: clang format complains again 🙈

UdjinM6

utACK 9d8bbd3

knst

LGTM 9d8bbd3

knst · 2024-09-10T14:03:50Z

src/coinjoin/client.cpp

@@ -47,6 +48,11 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataS
    CCoinJoinQueue dsq;
    vRecv >> dsq;

+    {
+        LOCK(cs_main);
+        Assert(peerman)->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));


What if by any reason CCoinJoinClientQueueManager will be initialized before peerman?

Should we just change assert here to if?

Suggested change

Assert(peerman)->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));

if (peerman) peerman->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));

knst · 2024-09-10T14:05:51Z

test/lint/lint-circular-dependencies.sh

@@ -87,10 +87,11 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
    "evo/deterministicmns -> validationinterface -> evo/deterministicmns"
    "logging -> util/system -> sync -> logging/timer -> logging"

+    "coinjoin/client -> net_processing -> coinjoin/client"


can you avoid adding new circulat dependencies?

These 2 disappeared just because they have shorter path between coinjoin/client.... coinjoin/client

"coinjoin/client -> coinjoin/util -> wallet/wallet -> psbt -> node/transaction -> node/context -> coinjoin/context -> coinjoin/client" "coinjoin/client -> coinjoin/coinjoin -> llmq/chainlocks -> net_processing -> coinjoin/client"

DSQ messages are 142 bytes. Previously, assuming a relatively highly connected masternode hosting 100 connection, each round of coinjoin will result in 14.2KB (100*142) of inbound and outbound traffic each. Now, using the inventory system, a message will first use 36 bytes per peer (sending and receiving), plus the size of a `getdata` message and the actual message itself. As a result, bandwidth usage for 1 round of mixing would be closer to 36 * 100 + 142 (dsq) + 36 (getdata) = ~3.8KB, a reduction of around ~73%

coolaj86 · 2024-11-07T18:45:48Z

Woah, where's the DIP for this?

Who voted to change the CoinJoin protocol?

I'm just about done with the Web CoinJoin wallet and y'all are changing protocols without any specs or voting?

…t processing / peerman dafa736 fix: respect SENDDSQUEUE message, move DSQ relay into net processing / peerman (pasta) Pull request description: ## Issue being fixed or feature implemented in #6148, I broke the functionality where a peer must opt in / opt out of DSQUEUE messages. This was mostly ok, and not immediately detected, as with this bug, simply everyone would receive DSQ messages over inventory (or classically, old proto versions were not affected by this bug). But this still would result in quite a bit of wasted bandwidth for peers which may not care about DSQ at all. ## What was done? This commit should restore the prior functionality, where a node should send the SENDDSQUEUE message if they wish to receive DSQs. Once they've sent that, depending on their protocol version, they will either have the messages pushed to them as available, or on modern protocols, they will thereafter receive DSQs over the inventory system. NOTE: I also refactor the code in this commit, moving some network proccessing into.... wait for it... net_processing.cpp! This allowed us to remove some dependencies in coinjoin.h. DSQ messages are now relayed to peers by calling peer_manager.RelayDSQ ## How Has This Been Tested? I have not yet mixed on testnet with this; we should include it in rc.2 and test ## Breaking Changes Slightly breaking for v22.0.x (so rc.1), as they in theory could be relying on this new logic of always receiving the DSQ inv. But I don't think anyone besides core is using this new protocol. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: light ACK dafa736 kwvg: utACK dafa736 Tree-SHA512: 18f9b0dfe05cde19db451653db9bb9a00352efd1bc37adffd83f74958010475f2782b1111b1c0d2dd967e7a851c3c4795fa55033b4bd0cc810aa293e754ce314

…into net processing / peerman dafa736 fix: respect SENDDSQUEUE message, move DSQ relay into net processing / peerman (pasta) Pull request description: ## Issue being fixed or feature implemented in dashpay#6148, I broke the functionality where a peer must opt in / opt out of DSQUEUE messages. This was mostly ok, and not immediately detected, as with this bug, simply everyone would receive DSQ messages over inventory (or classically, old proto versions were not affected by this bug). But this still would result in quite a bit of wasted bandwidth for peers which may not care about DSQ at all. ## What was done? This commit should restore the prior functionality, where a node should send the SENDDSQUEUE message if they wish to receive DSQs. Once they've sent that, depending on their protocol version, they will either have the messages pushed to them as available, or on modern protocols, they will thereafter receive DSQs over the inventory system. NOTE: I also refactor the code in this commit, moving some network proccessing into.... wait for it... net_processing.cpp! This allowed us to remove some dependencies in coinjoin.h. DSQ messages are now relayed to peers by calling peer_manager.RelayDSQ ## How Has This Been Tested? I have not yet mixed on testnet with this; we should include it in rc.2 and test ## Breaking Changes Slightly breaking for v22.0.x (so rc.1), as they in theory could be relying on this new logic of always receiving the DSQ inv. But I don't think anyone besides core is using this new protocol. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: light ACK dafa736 kwvg: utACK dafa736 Tree-SHA512: 18f9b0dfe05cde19db451653db9bb9a00352efd1bc37adffd83f74958010475f2782b1111b1c0d2dd967e7a851c3c4795fa55033b4bd0cc810aa293e754ce314

PastaPastaPasta added this to the 21.1 milestone Jul 24, 2024

PastaPastaPasta requested review from knst and UdjinM6 July 24, 2024 04:02

UdjinM6 requested changes Jul 24, 2024

View reviewed changes

src/protocol.h Show resolved Hide resolved

src/coinjoin/coinjoin.cpp Outdated Show resolved Hide resolved

UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024

PastaPastaPasta force-pushed the feat/send-dsq-over-inv branch from 7a4f608 to db59b0b Compare August 20, 2024 20:46

UdjinM6 requested changes Aug 21, 2024

View reviewed changes

src/coinjoin/coinjoin.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

UdjinM6 requested changes Aug 30, 2024

View reviewed changes

PastaPastaPasta self-assigned this Sep 4, 2024

PastaPastaPasta force-pushed the feat/send-dsq-over-inv branch from e85a31e to d6b49a0 Compare September 5, 2024 17:18

PastaPastaPasta requested a review from UdjinM6 September 5, 2024 17:18

UdjinM6 requested changes Sep 5, 2024

View reviewed changes

PastaPastaPasta requested a review from UdjinM6 September 6, 2024 19:23

UdjinM6 previously approved these changes Sep 6, 2024

View reviewed changes

PastaPastaPasta dismissed UdjinM6’s stale review via 9d8bbd3 September 6, 2024 20:15

UdjinM6 approved these changes Sep 6, 2024

View reviewed changes

knst approved these changes Sep 10, 2024

View reviewed changes

PastaPastaPasta force-pushed the feat/send-dsq-over-inv branch from 9d8bbd3 to 57fb087 Compare September 10, 2024 14:23

PastaPastaPasta merged commit 1464e69 into dashpay:develop Sep 10, 2024
7 checks passed

thephez added the P2P Some notable changes on p2p level label Sep 17, 2024

thephez mentioned this pull request Oct 7, 2024

docs(p2p): dsq updates dashpay/docs-core#121

Merged

PastaPastaPasta modified the milestones: 21.2, 22 Oct 29, 2024

PastaPastaPasta deleted the feat/send-dsq-over-inv branch November 19, 2024 13:59

PastaPastaPasta mentioned this pull request Nov 22, 2024

fix: respect SENDDSQUEUE message, move DSQ relay into net processing / peerman #6426

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: broadcast dsq messages using the inventory system #6148

feat: broadcast dsq messages using the inventory system #6148

PastaPastaPasta commented Jul 24, 2024

UdjinM6 left a comment

PastaPastaPasta commented Aug 20, 2024

PastaPastaPasta commented Aug 20, 2024

UdjinM6 commented Aug 21, 2024

PastaPastaPasta commented Aug 21, 2024

UdjinM6 commented Aug 21, 2024

PastaPastaPasta commented Aug 29, 2024

UdjinM6 Aug 30, 2024 •

edited

Loading

UdjinM6 Aug 30, 2024

UdjinM6 commented Aug 30, 2024

UdjinM6 left a comment

UdjinM6 left a comment

UdjinM6 left a comment

knst left a comment

knst Sep 10, 2024

knst Sep 10, 2024

coolaj86 commented Nov 7, 2024

	The DSQ message, starting in protocol version 70233, is broadcast using the inventory system, and not simply
	The DSQ message, starting in protocol version 70234, is broadcast using the inventory system, and not simply

	Assert(peerman)->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));
	if (peerman) peerman->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));

feat: broadcast dsq messages using the inventory system #6148

feat: broadcast dsq messages using the inventory system #6148

Conversation

PastaPastaPasta commented Jul 24, 2024

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Aug 20, 2024

PastaPastaPasta commented Aug 20, 2024

UdjinM6 commented Aug 21, 2024

PastaPastaPasta commented Aug 21, 2024

UdjinM6 commented Aug 21, 2024

PastaPastaPasta commented Aug 29, 2024

UdjinM6 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

UdjinM6 Aug 30, 2024

Choose a reason for hiding this comment

UdjinM6 commented Aug 30, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

knst Sep 10, 2024

Choose a reason for hiding this comment

knst Sep 10, 2024

Choose a reason for hiding this comment

coolaj86 commented Nov 7, 2024

UdjinM6 Aug 30, 2024 •

edited

Loading